whitelist command prefix integration in core and tui #7033
Conversation
zhao-oai
force-pushed
the
pr7032
branch
3 times, most recently
from
683bc5f to
1273f94
Compare
zhao-oai
force-pushed
the
pr7033
branch
4 times, most recently
from
9cb7ed0 to
80c63d3
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| commands: &[Vec<String>], | ||
| features: &Features, | ||
| ) -> Option<Vec<String>> { | ||
| if features.enabled(Feature::ExecPolicy) && commands.len() == 1 { |
There was a problem hiding this comment.
I don't love the solution I came up with here w.r.t only persisting allow prefix if ExecPolicy is enabled. I think a better solution would be to change the way we store Policy in session state. We should store either an enum indicating whether ExecPolicy is enabled or store Policy as an optional. However, I think this is fit for a followup PR
zhao-oai
changed the title
zhao-oai
changed the title
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| NeedsApproval { reason: Option<String> }, | ||
| NeedsApproval { | ||
| reason: Option<String>, | ||
| allow_prefix: Option<Vec<String>>, |
There was a problem hiding this comment.
and will this setup still make sense once we flesh out execpolicy to support the more complex rule type?
There was a problem hiding this comment.
and will this setup still make sense once we flesh out execpolicy to support the more complex rule type?
i think so! even with more complex rule types, i imagine we'll still want to whitelist command prefix for the "approve and don't ask again" option in the TUI
| features: Features, | ||
| /// Execpolicy policy, applied only when enabled by feature flag. | ||
| exec_policy: Arc<ExecPolicy>, | ||
| exec_policy: Arc<RwLock<ExecPolicy>>, |
There was a problem hiding this comment.
Hmm, this feels slightly undesirable since SessionConfiguration is already generally wrapped in a mutex, but maybe this is OK, I'm not sure yet...
| } | ||
|
|
||
| #[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)] | ||
| pub struct ExecApprovalRequestEvent { |
There was a problem hiding this comment.
We will need to update the v2 app server protocol as well, I expect. /cc @owenlin0
There was a problem hiding this comment.
@zhao-oai yes that'd be great if we can keep app-server at feature-parity (but can be done as a followup PR). We'd probably want to add a field here: https://github.com/openai/codex/blob/main/codex-rs/app-server-protocol/src/protocol/v2.rs#L1428 and wire it up
this PR enables TUI to approve commands and add their prefixes to an allowlist:
note: we only show the option to whitelist the command when
git add -A && git commit -m 'hello world')